Skip to content

Conversation

@kellyguo11
Copy link
Contributor

Description

Adding some additional app settings to improve viewport rendering performance for non-headless workflows.

Fixes #3548

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@github-actions github-actions bot added the isaac-sim Related to Isaac Sim team label Oct 15, 2025
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 16, 2025
@kellyguo11 kellyguo11 marked this pull request as draft October 16, 2025 17:22
@kellyguo11 kellyguo11 marked this pull request as ready for review October 22, 2025 03:32
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Greptile Summary

This PR improves viewport rendering performance for non-headless workflows by adding three app settings to apps/isaaclab.python.kit: enabling partial GPU updates (partialGpuUpdate = 1), disabling rate limiting (rateLimitEnabled = false), and enabling manual loop mode (manualModeEnabled = true). It also adds the omni.kit.loop-isaac dependency to support manual loop control and removes the useFixedTimeStepping setting. Additionally, it documents a known Isaac Sim 5.0 performance regression in the release notes.

PR Description Notes:

  • The PR description indicates checkboxes for tests and changelog updates are unchecked, but these should be completed before merging

Potential Issues:

  1. Missing Tests: The PR lacks tests to verify the performance improvements or ensure the new settings don't break existing functionality. This is particularly important since these changes affect core rendering behavior and could impact simulation determinism.

  2. No Changelog Update: The checklist shows the changelog hasn't been updated, despite this being a user-facing configuration change that affects application behavior. This should be documented in source/extensions/omni.isaac.lab/docs/CHANGELOG.rst.

  3. Removal of useFixedTimeStepping: Removing player.useFixedTimeStepping = true without explanation could affect physics simulation reproducibility and determinism in RL training workflows. This needs explicit documentation about why it's safe to remove and what implications it has for users relying on fixed time-stepping.

  4. Manual Loop Mode Concerns: Enabling manualModeEnabled = true gives direct control over the render loop, which could introduce timing-related issues or race conditions in multi-threaded environments. The interaction with Isaac Lab's existing simulation loop management needs verification.

  5. Headless Workflow Impact: While the PR states these changes are "for non-headless workflows," the configuration file isaaclab.python.kit appears to be used broadly. There's no conditional logic to ensure these rendering optimizations only apply to non-headless scenarios, which could unintentionally affect headless performance.

Confidence: 3/5 - The changes are straightforward configuration updates, but the lack of tests, missing changelog, unexplained removal of useFixedTimeStepping, and potential impact on simulation determinism create moderate risk. The performance improvements are likely beneficial, but validation is needed to ensure no regressions in physics accuracy or training reproducibility.

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@kellyguo11 kellyguo11 marked this pull request as draft October 26, 2025 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation isaac-sim Related to Isaac Sim team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant